Repository navigation
[Config] remove duplicated csv row - #6332
Conversation
🏷️ CI GuideRuns automatically on every PR:
Extended tests (opt-in via labels):
One backend per PR: PR title tags & labels: |
The duplicate-shape check only ran when a family had more than one table to merge. Two short-circuits kept single-file families out of it entirely: get_config_file returns the canonical CSV untouched when model_configs/ has no sibling, and update_config_files returns early on a one-element path list. A family that never grows a per-model table was therefore never validated, and a lone CSV can still hold two rows with the same key when a retune is appended instead of replacing the old row. aiter/configs/tuned_grouped_fmoe.csv was in exactly that state: two rows for gfx1250 token=512 model_dim=7168 inter_dim=3072 differing only in tuning parameters. The row was removed in ROCm#6332, but the mechanism that let it through was not, and test_grouped_fmoe stayed green throughout because the resolution path it drives never reached the check. Extract the check into _raise_on_duplicate_shapes and call it from both paths, so the key derivation, the error text, and the keep-lowest-us write-back stay in one place. env_name is threaded through, so the paths reached for the first time still name the AITER_CONFIG_* variable. Only the single-file duplicate case changes behavior: it is now reported instead of silently accepted. Multi-file merges, and single files that are clean, missing, header-only or unparseable, resolve as before. _Mxfp8Config.update_config_files keeps its own single-path short-circuit; that op validates individual rows in its loader, so its behavior does not change. Signed-off-by: yzhou103 <yzhou103@amd.com>
The self-checks only drove the cross-file merge: _build_synthetic_family always wrote a model_configs/ sibling, so the path where a family has no sibling and resolves to its canonical CSV directly was never exercised. That is the path that skipped the duplicate check, which is how tuned_grouped_fmoe.csv kept a duplicate row (removed in ROCm#6332) while every test here was green. Removing the row makes test_grouped_fmoe green again without proving the check runs, so add controls that do not depend on repository data: - a duplicate planted in a lone CSV must be reported - two distinct shapes in a lone CSV must not be - an unparseable lone CSV must not raise out of get_config_file, since this path used to hand the file back unread and the parse error surfaced from the op's own loader _build_synthetic_family takes with_model_file to choose the path and canonical_raw for malformed input. _run_synthetic now catches Exception rather than RuntimeError: pandas' ParserError is a ValueError, so the malformed case has to be observable rather than escaping as a test error. Verified by mutation: reverting the single-file check fails only the first, and dropping the parse guard fails only the third. Signed-off-by: yzhou103 <yzhou103@amd.com>
* [Config] Check single-file tuned configs for duplicate shapes The duplicate-shape check only ran when a family had more than one table to merge. Two short-circuits kept single-file families out of it entirely: get_config_file returns the canonical CSV untouched when model_configs/ has no sibling, and update_config_files returns early on a one-element path list. A family that never grows a per-model table was therefore never validated, and a lone CSV can still hold two rows with the same key when a retune is appended instead of replacing the old row. aiter/configs/tuned_grouped_fmoe.csv was in exactly that state: two rows for gfx1250 token=512 model_dim=7168 inter_dim=3072 differing only in tuning parameters. The row was removed in #6332, but the mechanism that let it through was not, and test_grouped_fmoe stayed green throughout because the resolution path it drives never reached the check. Extract the check into _raise_on_duplicate_shapes and call it from both paths, so the key derivation, the error text, and the keep-lowest-us write-back stay in one place. env_name is threaded through, so the paths reached for the first time still name the AITER_CONFIG_* variable. Only the single-file duplicate case changes behavior: it is now reported instead of silently accepted. Multi-file merges, and single files that are clean, missing, header-only or unparseable, resolve as before. _Mxfp8Config.update_config_files keeps its own single-path short-circuit; that op validates individual rows in its loader, so its behavior does not change. Signed-off-by: yzhou103 <yzhou103@amd.com> * [Config] Cover the single-file path in the collision self-checks The self-checks only drove the cross-file merge: _build_synthetic_family always wrote a model_configs/ sibling, so the path where a family has no sibling and resolves to its canonical CSV directly was never exercised. That is the path that skipped the duplicate check, which is how tuned_grouped_fmoe.csv kept a duplicate row (removed in #6332) while every test here was green. Removing the row makes test_grouped_fmoe green again without proving the check runs, so add controls that do not depend on repository data: - a duplicate planted in a lone CSV must be reported - two distinct shapes in a lone CSV must not be - an unparseable lone CSV must not raise out of get_config_file, since this path used to hand the file back unread and the parse error surfaced from the op's own loader _build_synthetic_family takes with_model_file to choose the path and canonical_raw for malformed input. _run_synthetic now catches Exception rather than RuntimeError: pandas' ParserError is a ValueError, so the malformed case has to be observable rather than escaping as a test error. Verified by mutation: reverting the single-file check fails only the first, and dropping the parse guard fails only the third. Signed-off-by: yzhou103 <yzhou103@amd.com> * [Config] Keep the duplicate diagnostic when a source cannot be rewritten The dedup write-back runs before the RuntimeError that reports the duplicates, so a source that cannot be written replaces that diagnostic with a bare PermissionError. That drops the dedup key and the AITER_CONFIG_* hint precisely in a read-only install or container, which is the environment those were added for (#6307, #5184), and abandons any remaining source half-rewritten. Catch OSError per source, collect the failures, and report them under "Not updated" without counting them as updated. The resolution line now says not every file could be rewritten when that happened, so the message cannot claim a write that did not land. This affected merges before and now also affects lone config files, since those reach the same write-back. Reported by Copilot review. Signed-off-by: yzhou103 <yzhou103@amd.com> --------- Signed-off-by: yzhou103 <yzhou103@amd.com>
Motivation
Technical Details
Test Plan
Test Result
Submission Checklist